fix: callback with error if SyncWriteStream writeSync failed by killagu · Pull Request #47949 · nodejs/node

killagu · 2023-05-10T11:48:40Z

lib/internal/fs/sync_write_stream.js

bnoordhuis · 2023-05-11T00:57:47Z

General observation: the way _write() synchronously invokes cb() is wrong(ish). Node normally follows the iron-clad rule that callbacks are always invoked asynchronously, otherwise you can end up overflowing the stack. Ex.:

function again() {
  stream._write(buf, 'utf8', again); // RangeError: Maximum call stack size exceeded 
}

edit: oh nevermind, this is stream.Writable._write() and the streams code takes care of deferring the callback.

nodejs-github-bot · 2023-05-12T22:28:18Z

CI: https://ci.nodejs.org/job/node-test-pull-request/51767/

killagu · 2023-05-23T14:50:52Z

ping @bnoordhuis

test/parallel/test-internal-fs-syncwritestream.js

nodejs-github-bot · 2023-06-21T06:25:16Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52303/

nodejs-github-bot · 2023-06-21T07:35:46Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52305/

nodejs-github-bot · 2023-06-21T16:11:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52321/

lpinca · 2023-06-21T18:19:03Z

Some failures on Windows seems related. @killagu can you take a look?

killagu · 2023-06-21T23:53:42Z

Of course.

killagu · 2023-06-25T13:13:32Z

Sorry, I'm still looking for a suitable Windows machine to run Node.js compilation and testing. I will provide an update once there is progress.

killagu · 2023-06-26T07:39:31Z

@lpinca I've identified the cause. On the Windows platform, it requires manual release of the fd; otherwise, it will block the process from cleaning up the temporary directory upon exiting. Could you please trigger the CI again? Thank you.

nodejs-github-bot · 2023-06-26T07:40:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52484/

killagu · 2023-06-26T07:52:56Z

😅 Force push let the ci down fatal: reference is not a tree: 7bcb45c7a81c3bba6e9ffa96754329c9facd4aa4.

nodejs-github-bot · 2023-06-26T08:02:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52485/

lib/internal/fs/sync_write_stream.js

lpinca · 2023-06-26T10:30:42Z

Also one final request. Can you please change the commit message to something like this?

fs: call the callback with an error if writeSync fails

Thank you.

Catch SyncWriteStream write file error. Fixes: nodejs#47948 Signed-off-by: killagu <killa123@126.com>

killagu · 2023-06-26T11:16:56Z

It's updated. Thanks very much for review and lots of help.

nodejs-github-bot · 2023-06-26T11:43:23Z

CI: https://ci.nodejs.org/job/node-test-pull-request/52489/

nodejs-github-bot · 2023-06-26T14:19:00Z

Landed in 32eb492

Catch SyncWriteStream write file error. Fixes: #47948 Signed-off-by: killagu <killa123@126.com> PR-URL: #47949 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Catch SyncWriteStream write file error. Fixes: nodejs#47948 Signed-off-by: killagu <killa123@126.com> PR-URL: nodejs#47949 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

Catch SyncWriteStream write file error. Fixes: #47948 Signed-off-by: killagu <killa123@126.com> PR-URL: #47949 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels May 10, 2023

killagu commented May 10, 2023

View reviewed changes

lib/internal/fs/sync_write_stream.js Show resolved Hide resolved

killagu force-pushed the fix/stdout_error branch from 108e831 to c5b5375 Compare May 10, 2023 12:00

killagu force-pushed the fix/stdout_error branch 3 times, most recently from 3726f80 to 7c8c81f Compare May 11, 2023 07:39

kvakil added the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 12, 2023

This was referenced May 13, 2023

CI Reliability 2023-05-13 nodejs/reliability#564

Open

CI Reliability 2023-05-14 nodejs/reliability#565

Open

killagu force-pushed the fix/stdout_error branch 2 times, most recently from c38701c to eff6f0f Compare May 14, 2023 01:32

This was referenced May 15, 2023

CI Reliability 2023-05-15 nodejs/reliability#566

Open

CI Reliability 2023-05-16 nodejs/reliability#567

Open

CI Reliability 2023-05-17 nodejs/reliability#568

Open

CI Reliability 2023-05-18 nodejs/reliability#569

Open

fengmk2 mentioned this pull request May 30, 2023

egg服务出现 EIO 报错，导致服务停止运行 eggjs/egg#4967

Open

lpinca approved these changes Jun 17, 2023

View reviewed changes

test/parallel/test-internal-fs-syncwritestream.js Outdated Show resolved Hide resolved

lpinca approved these changes Jun 20, 2023

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 21, 2023

github-actions bot mentioned this pull request Jun 24, 2023

CI Reliability 2023-06-24 nodejs/reliability#597

Open

25 tasks

killagu force-pushed the fix/stdout_error branch from 8c4b1af to 7bcb45c Compare June 26, 2023 07:35

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2023

killagu force-pushed the fix/stdout_error branch from 7bcb45c to 021f107 Compare June 26, 2023 07:41

lpinca approved these changes Jun 26, 2023

View reviewed changes

lib/internal/fs/sync_write_stream.js Outdated Show resolved Hide resolved

fs: call the callback with an error if writeSync fails

927888b

Catch SyncWriteStream write file error. Fixes: nodejs#47948 Signed-off-by: killagu <killa123@126.com>

killagu force-pushed the fix/stdout_error branch from dd12bf0 to 927888b Compare June 26, 2023 11:15

lpinca approved these changes Jun 26, 2023

View reviewed changes

lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 26, 2023

lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 26, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 26, 2023

nodejs-github-bot merged commit 32eb492 into nodejs:main Jun 26, 2023

RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023

fs: call the callback with an error if writeSync fails

19fde63

Catch SyncWriteStream write file error. Fixes: #47948 Signed-off-by: killagu <killa123@126.com> PR-URL: #47949 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

RafaelGSS mentioned this pull request Jul 3, 2023

v20.4.0 proposal #48643

Merged

ruyadorno mentioned this pull request Sep 10, 2023

v18.18.0 proposal #49220

Merged

Uh oh!

Conversation

killagu commented May 10, 2023

Uh oh!

Uh oh!

bnoordhuis commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented May 12, 2023

Uh oh!

killagu commented May 23, 2023

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 21, 2023

Uh oh!

nodejs-github-bot commented Jun 21, 2023

Uh oh!

nodejs-github-bot commented Jun 21, 2023

Uh oh!

lpinca commented Jun 21, 2023

Uh oh!

killagu commented Jun 21, 2023

Uh oh!

killagu commented Jun 25, 2023

Uh oh!

killagu commented Jun 26, 2023

Uh oh!

nodejs-github-bot commented Jun 26, 2023

Uh oh!

killagu commented Jun 26, 2023

Uh oh!

nodejs-github-bot commented Jun 26, 2023

Uh oh!

Uh oh!

lpinca commented Jun 26, 2023

Uh oh!

killagu commented Jun 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 26, 2023

Uh oh!

nodejs-github-bot commented Jun 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

bnoordhuis commented May 11, 2023 •

edited

Loading

killagu commented Jun 26, 2023 •

edited

Loading